Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

repl: make last error available as _error #18919

Closed
wants to merge 3 commits into from
Closed

repl: make last error available as _error #18919

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

This is pretty useful when trying to inspect the last
error caught by a REPL, and is made to be analogous to _,
which contains the last successful completion value.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Feb 21, 2018
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/16425/

evanlucas
evanlucas approved these changes Feb 21, 2018
View reviewed changes
doc/api/repl.md Outdated
@@ -162,6 +162,17 @@ Expression assignment to _ now disabled.
4
```

Similarly, `_err` will refer to the last seen error, if there was any.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we could add a Changes section for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanlucas done!

@Fishrock123
Copy link
Contributor

Would it be a good idea to make it something... less likely to conflict? __err or a symbol?

@addaleax
Copy link
Member Author

@Fishrock123 How would _err occur naturally in the REPL? I’m open to bikeshedding for the name, but a Symbol would defeat the purpose of making this value easily accessible.

vdeturckheim
vdeturckheim approved these changes Feb 21, 2018
View reviewed changes
Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2018
@jasnell
Copy link
Member

jasnell commented Feb 22, 2018

Love the idea, needs a better name. Two underscores is better than one and would prefer something like __lastError (that is, slightly more descriptive and not abbreviated)

@addaleax
Copy link
Member Author

addaleax commented Feb 22, 2018 •
edited
Loading

@jasnell I think the main goal for the name is that it’s supposed to be typable really, really easily and quickly.

I love verbose names in real code, because we’re writing it for the 100 readers coming after us, but the REPL has the exact opposite goal of that: It’s code that’s only written once, ever, and never read by anybody else more than a minute after it was written. It’s not about choosing good or descriptive variable names in any way, it’s about being easy to write, so I would really prefer to keep it short and succinct.

(Literally all REPLs that I know of use _ or ans for the last value for those very reasons.)

The only thing I’m worried about here is discoverability, because right now you’d have to hear about it somewhere or read the docs. I’ve gone with _err because that’s one of the things I’d type to see whether this feature exists; __lastError is not on that list.

I’m also not worried about collisions; there is an explicit warning text being emitted here if that happens, like it is for _.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2018
tniessen
tniessen approved these changes Feb 23, 2018
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with either name.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

@addaleax ... then how about meeting in the middle with _error?

@addaleax
Copy link
Member Author

@jasnell I’m fine with that, but honestly, I’m just going to write _err<TAB> every time. 😄

I’ve switched it for now but it might be helpful to know what your concerns actually are.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

The concern is that I'm not particularly fond of abbreviated things.

@TimothyGu
Copy link
Member

How about $err? The $ prefix would be consistent with other command line APIs.

@addaleax
repl: make last error available as _error
ac6f5dd 
This is pretty useful when trying to inspect the last
error caught by a REPL, and is made to be analogous to `_`,
which contains the last successful completion value.
@addaleax
[squash] add changes: section
fb8b01f
@addaleax
[squash] switch to error
1867690
@addaleax addaleax changed the title repl: make last error available as _err repl: make last error available as _error Feb 26, 2018
@addaleax
Copy link
Member Author

@TimothyGu I think @jasnell would want that to be $error ;) I would kind of like to be consistent with _ too, though…

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/13431/

@addaleax
Copy link
Member Author

addaleax commented Mar 2, 2018

It seems like the CI run here was started for a different PR.

CI: https://ci.nodejs.org/job/node-test-commit/16599/

@addaleax
Copy link
Member Author

addaleax commented Mar 4, 2018

Landed in a8b5192

@addaleax addaleax closed this Mar 4, 2018
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018
addaleax added a commit that referenced this pull request Mar 4, 2018
@addaleax
repl: make last error available as _error
a8b5192 
This is pretty useful when trying to inspect the last
error caught by a REPL, and is made to be analogous to `_`,
which contains the last successful completion value.

PR-URL: #18919
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Mar 5, 2018
@addaleax
repl: make last error available as _error
96f0bec 
This is pretty useful when trying to inspect the last
error caught by a REPL, and is made to be analogous to `_`,
which contains the last successful completion value.

PR-URL: nodejs#18919
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label May 2, 2018
@addaleax addaleax deleted the repl-last-error branch May 2, 2018 16:32
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
repl: make last error available as _error
c2e76a7 
This is pretty useful when trying to inspect the last
error caught by a REPL, and is made to be analogous to `_`,
which contains the last successful completion value.

PR-URL: nodejs#18919
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@vdeturckheim vdeturckheim vdeturckheim approved these changes

@devsnek devsnek devsnek approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@princejwesley princejwesley princejwesley approved these changes

@Leko Leko Leko approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.